@claude review in regards to the last 20 commits in master, and suggest fixes

Claude finished @arr2036's task in 5m 59s —— View job

Review

• Read PR diff vs master
• Examine the 20 recent master commits for context
• Read changed files in detail
• Identify issues and suggest fixes

Reviewed a16694f ("New profiling images based off of crossbuild images") in light of the 20 most-recent master commits — in particular the m4 refactor (24cddf6 factor common build setup into shared m4 snippets, add ci-base template), the pre-baked-image work (fe4b22c bake CI build environments into pre-baked base images), and the m4 path fix (d74e581 crossbuild: fix double-slash in m4 paths).

Critical

1. $APT_OPTS is undefined inside Dockerfile.prof. ARG APT_OPTS=... lives in crossbuild.deb.m4 (the base image build). profiling.deb.m4 starts a brand new build stage with FROM ${from} and never redeclares it, so in scripts/docker/m4/profiling.deb.m4:13,34,60,72,101 apt-get install -y $APT_OPTS ... expands to apt-get install -y — no --no-install-recommends, no --force-unsafe-io. The first RUN (kcachegrind + KDE/Qt libs) will pull in a huge recommended-package set as a result. Either redeclare ARG APT_OPTS=... after FROM, or drop the $APT_OPTS references and write the flags out explicitly. Fix this →

2. The makefile rules are emitted for every distro, but the m4 is Ubuntu-only. crossbuild.mk does $(foreach IMAGE,$(CB_IMAGES), $(eval ...)) which generates crossbuild.rocky9.profile.regen, crossbuild.debian12.profile.regen, etc. profiling.deb.m4 hard-codes ubuntu-dbgsym-keyring, ddebs.ubuntu.com, and t64-suffixed Ubuntu-24 dbgsym names (libreadline8t64-dbgsym, libssl3t64-dbgsym, …). Running the regen target for any non-Ubuntu-24 image produces a Dockerfile that will fail to build. Either:

• Gate the rule expansion on a per-image whitelist (e.g. ifeq on D_NAME/PKG_TYPE), or
• Add ifelse(D_NAME, ...) branches inside profiling.deb.m4 for rocky/debian (same pattern as the debian10/debiansid blocks in crossbuild.deb.m4).

3. PR doesn't use the new shared m4 snippets. Master commit 24cddf6 introduced common.apt.retries.m4, common.deb.toolchain.m4, common.deb.nr-extras.m4. profiling.deb.m4 duplicates apt-get update patterns and gets no retry handling — apt flakiness in CI will surface here. Compare ci-base.deb.m4:6-7 which simply does include('common.apt.retries.m4')dnl. Worth include-ing the same snippets here.

Significant

4. PROFILE_NAME is plumbed through m4 but never used. crossbuild.mk:291 passes -D PROFILE_NAME=$(PROFILE), but profiling.deb.m4 contains no PROFILE_NAME reference. Either drop the -D or use it (e.g. ENV PROFILE_NAME=PROFILE_NAME so runtime tooling can identify itself).

5. Generated header points at a non-existent make target. Dockerfile.prof:4 reads # Rebuild this file with make profiling.ubuntu24.regen, but no such target exists — the real target is crossbuild.ubuntu24.profile.regen. The header comes from Dockerfile.m4's make D_TYPE.D_NAME.regen placeholder, which assumes D_TYPE.D_NAME.regen is also the real target name. Either rename the makefile target to profiling.${1}.regen (top-level, sibling of crossbuild) or override the header for this template.

6. profile.build has no dependency on the base crossbuild stamp. crossbuild.mk:162 declares $(DD)/stamp-image.${1}-profile.build: $(DT)/${1}/Dockerfile.prof — but Dockerfile.prof does FROM freeradius40x-build/ubuntu24. On a clean checkout, make crossbuild.ubuntu24.profile.build will fail with image not found because nothing has built freeradius40x-build/ubuntu24 yet. Add $(DD)/stamp-image.${1} as a prerequisite. Fix this →

7. profile.reset doesn't remove the docker image. crossbuild.mk:298 only rm -fs the stamp. Without docker image rm freeradius4-$(PROFILE)/${1} the next profile.build reuses the cached image and the reset has no observable effect unless the user also passes NOCACHE=1. Either add the docker image rm, or split into profile.reset (stamp only) + profile.distclean (image too), mirroring the base image's reset vs distclean.

8. Inconsistent image-prefix naming. Base uses CB_IPREFIX:=freeradius40x-build. Profiling hard-codes freeradius4-$(PROFILE)/${1}. If freeradius4- is deliberate (it implies "FreeRADIUS 4.x"), introduce a PROF_IPREFIX:=freeradius4 (or derive from CB_IPREFIX) so the next major version doesn't require touching multiple places.

Minor / nits

9. Image bloat. cargo install inferno drags in the full Rust toolchain just to ship one binary; consider a multi-stage build that copies only the final /usr/local/bin/inferno-* out and discards cargo. Similarly, kcachegrind + KDE/Qt is hundreds of MB of GUI libraries inside what's logically a server container — these are typically used on a developer workstation reading exported callgrind output. Worth checking whether they need to live in the image at all.

10. Non-reproducible clones. git clone --depth 1 https://github.com/mheily/libkqueue.git and git clone --depth 1 https://github.com/brendangregg/FlameGraph aren't pinned. Pin to a release tag or commit SHA so two builds a month apart aren't subtly different. inferno --version 0.11.21 --locked already does this correctly.

11. rm -r /var/lib/apt/lists/* will fail on empty glob. Use rm -rf. Several other m4 files have the same pattern, so it's a pre-existing convention rather than a regression — but worth fixing here while you're touching the file.

12. Stamp name uses mixed separators. stamp-image.${1}-profile.build mixes - and .. Existing rules use stamp-image.${1}. stamp-image.${1}.profile (without the .build suffix) reads more like the rest of the codebase.

13. No CI workflow consumes these targets. This matches @alandekok's "developer convenience" intent (per the discussion on the help text), but it's worth confirming explicitly. If profiling images are ever wired into CI, follow the fe4b22c/24cddf6 pre-baked-image pattern rather than rebuilding the image inside every CI run.

Review-comment status

The thread cleanup from earlier rounds looks resolved:

• default-profiling naming adopted ✓
• profiles/default-profiling/{README,start,configure}.sh removed ✓
• m4-path / filter profiling no longer needed since profiling lives outside build/ … but note: scripts/docker/build/ubuntu24/Dockerfile.prof is inside build/, which contradicts mcnewton's premise. Worth checking whether anything in docker.mk still globs build/*/Dockerfile* and would now pick up Dockerfile.prof unintentionally.
  • branch new-profiling-image-build

I've merged a set of changes derived from these, but with the duplication removed. There are now common macros which handled rendering docker files, and image lifecycles.

These are the new targets:

  - dockerfile.profiling — regen every Dockerfile.profiling
  - dockerfile.profiling.check — fail if any Dockerfile.profiling is stale
  - docker.profiling — build every profiling image
  - docker.profiling.up — docker run a sleep-forever container for every profiling image
  - docker.profiling.down — stop every profiling container
  - docker.profiling.reset — rm stamps for every profiling image
  - docker.profiling.clean — rmi + rm stamps for every profiling image (full nuke)

There's also docker.profiling.IMAGE.VERB targets. Where image is one of debian12 debian13 debiansid rocky9 rocky10 ubuntu22 ubuntu24 ubuntu26